Make sure that the data types are supported in hashjoin before genera… #2702

HuSen8891 · 2022-06-06T02:59:27Z

Which issue does this PR close?

Rationale for this change

Before generating the hash join logical plan, make sure the data types in equal conditions are supported. Otherwise, try
cross join instead.

What changes are included in this PR?

add can_hash func in datafusion/expr/src/utils.rs
use can_hash in datafusion/expr/src/logical_plan/builder.rs and datafusion/sql/src/planner.rs to check if the data type is supported in hashjoin or not

…ting hashjoin logical plan. If data types are not supported in hashjoin, try cross join with filters.

codecov-commenter · 2022-06-06T05:40:14Z

Codecov Report

Merging #2702 (b9b9434) into master (9306534) will decrease coverage by 0.01%.
The diff coverage is 79.06%.

@@            Coverage Diff             @@
##           master    #2702      +/-   ##
==========================================
- Coverage   84.66%   84.64%   -0.02%     
==========================================
  Files         270      270              
  Lines       46919    47037     +118     
==========================================
+ Hits        39724    39815      +91     
- Misses       7195     7222      +27

Impacted Files	Coverage Δ
datafusion/expr/src/logical_plan/builder.rs	`89.35% <55.55%> (-2.60%)`	⬇️
datafusion/expr/src/utils.rs	`90.39% <68.18%> (-1.48%)`	⬇️
datafusion/sql/src/planner.rs	`81.33% <69.23%> (-0.19%)`	⬇️
datafusion/core/tests/sql/joins.rs	`99.07% <100.00%> (+0.13%)`	⬆️
datafusion/row/src/lib.rs	`100.00% <0.00%> (ø)`
datafusion/row/src/layout.rs	`98.27% <0.00%> (ø)`
datafusion/row/src/reader.rs	`88.27% <0.00%> (ø)`
datafusion/row/src/writer.rs	`89.86% <0.00%> (ø)`
datafusion/row/src/accessor.rs	`56.41% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9306534...b9b9434. Read the comment docs.

andygrove · 2022-06-06T16:29:57Z

datafusion/expr/src/logical_plan/builder.rs

+                    Some(filter_expr) => {
+                        filters = Some(Expr::BinaryExpr {
+                            left: Box::new(expr),
+                            op: Operator::And,
+                            right: Box::new(filter_expr),
+                        });
+                    }


Suggested change

Some(filter_expr) => {

filters = Some(Expr::BinaryExpr {

left: Box::new(expr),

op: Operator::And,

right: Box::new(filter_expr),

});

}

Some(filter_expr) => filters = Some(and(expr, filter_expr)),

Thank you! I'll fix this.

andygrove · 2022-06-06T16:31:08Z

datafusion/expr/src/logical_plan/builder.rs

+                let expr = Expr::BinaryExpr {
+                    left: Box::new(Expr::Column(l.clone())),
+                    op: Operator::Eq,
+                    right: Box::new(Expr::Column(r.clone())),
+                };


Suggested change

let expr = Expr::BinaryExpr {

left: Box::new(Expr::Column(l.clone())),

op: Operator::Eq,

right: Box::new(Expr::Column(r.clone())),

};

let expr = binary_expr(

Expr::Column(l.clone()),

Operator::Eq,

Expr::Column(r.clone()),

);

Thank you! I'll fix this.

andygrove · 2022-06-06T16:31:48Z

datafusion/expr/src/logical_plan/builder.rs

@@ -19,6 +19,7 @@

 use crate::expr_rewriter::{normalize_col, normalize_cols, rewrite_sort_cols_by_aggs};
 use crate::utils::{columnize_expr, exprlist_to_fields, from_plan};
+use crate::Operator;


I left some suggestions below that require these additional imports

Suggested change

use crate::Operator;

use crate::{and, binary_expr, Operator};

Thank you! I'll fix this.

liukun4515 · 2022-06-07T02:26:39Z

datafusion/expr/src/utils.rs

+/// If more data types are supported in hash join, add those data types here
+/// to generate join logical plan.
+pub fn can_hash(data_type: &DataType) -> bool {
+    match data_type {


decimal should be supported here

Data types here come from function equal_rows, decimal is not supported in equal_rows so that hash join currently does not support joining on columns of decimal data type. That's why decimal is not here.

I think it may help to add a comment here (or in equal_rows) mentioning they need to remain in sync

I think it may help to add a comment here (or in equal_rows) mentioning they need to remain in sync

I'll add this comment.

alamb

Thank you @AssHero

I believe this PR does what the description says -- and it adds nice test coverage so I am approving it.

However, I am concerned that trying to run a query using a CrossJoin is likely to be catastrophically bad so this approach will not work for anything except very small inputs.

alamb · 2022-06-08T19:55:54Z

datafusion/core/tests/sql/joins.rs

+    // join on hash unsupported data type (Date32), use cross join instead hash join
+    let sql = "select * from foo t1 join foo t2 on t1.c4 = t2.c4";
+    let msg = format!("Creating logical plan for '{}'", sql);


So I think CrossJoin is almost never what the user would want: as once the tables get beyond any trivial size the query will effectively never finish or will run out of memory. An error is clearer.

From the issue description #2145 (comment) I think @pjmore's idea to cast unsupported types to a supported type is a good one -- the arrow cast kernels are quite efficient for things like Date32 -> Int32 (no copies) as the representations are the same

@pjmore what do you think?

So I think CrossJoin is almost never what the user would want: as once the tables get beyond any trivial size the query will effectively never finish or will run out of memory. An error is clearer.

From the issue description #2145 (comment) I think @pjmore's idea to cast unsupported types to a supported type is a good one -- the arrow cast kernels are quite efficient for things like Date32 -> Int32 (no copies) as the representations are the same

@pjmore what do you think?

I agree. supporting more data types in hash join is the better way to solve this issue, and i'm already working on it.
And this pr only wants to make hash unsupported join running in cross join instead of error/panic, we can support more data types in hash join continuously.

alamb · 2022-06-08T19:59:03Z

datafusion/expr/src/utils.rs

+/// If more data types are supported in hash join, add those data types here
+/// to generate join logical plan.
+pub fn can_hash(data_type: &DataType) -> bool {
+    match data_type {


I think it may help to add a comment here (or in equal_rows) mentioning they need to remain in sync

alamb · 2022-06-09T10:41:12Z

Thank you again @AssHero

Make sure that the data types are supported in hashjoin before genera…

cc56f63

…ting hashjoin logical plan. If data types are not supported in hashjoin, try cross join with filters.

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner labels Jun 6, 2022

HuSen8891 added 2 commits June 6, 2022 11:49

format the expected output of join test case

06569b2

refine the code

9d74954

andygrove reviewed Jun 6, 2022

View reviewed changes

refine the code according to code review's comments

b9b9434

liukun4515 reviewed Jun 7, 2022

View reviewed changes

HuSen8891 requested a review from andygrove June 8, 2022 00:18

alamb approved these changes Jun 8, 2022

View reviewed changes

add comments

ea06ab5

alamb merged commit 67d91a7 into apache:master Jun 9, 2022

HuSen8891 deleted the stat branch June 9, 2022 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure that the data types are supported in hashjoin before genera… #2702

Make sure that the data types are supported in hashjoin before genera… #2702

HuSen8891 commented Jun 6, 2022

codecov-commenter commented Jun 6, 2022 •

edited

Loading

andygrove Jun 6, 2022

HuSen8891 Jun 7, 2022

andygrove Jun 6, 2022

HuSen8891 Jun 7, 2022

andygrove Jun 6, 2022 •

edited

Loading

HuSen8891 Jun 7, 2022

liukun4515 Jun 7, 2022

HuSen8891 Jun 7, 2022 •

edited

Loading

alamb Jun 8, 2022

HuSen8891 Jun 9, 2022

alamb left a comment

alamb Jun 8, 2022

HuSen8891 Jun 9, 2022 •

edited

Loading

alamb Jun 8, 2022

alamb commented Jun 9, 2022

	use crate::Operator;
	use crate::{and, binary_expr, Operator};

Make sure that the data types are supported in hashjoin before genera… #2702

Make sure that the data types are supported in hashjoin before genera… #2702

Conversation

HuSen8891 commented Jun 6, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

codecov-commenter commented Jun 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSen8891 Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSen8891 Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jun 9, 2022

codecov-commenter commented Jun 6, 2022 •

edited

Loading

andygrove Jun 6, 2022 •

edited

Loading

HuSen8891 Jun 7, 2022 •

edited

Loading

HuSen8891 Jun 9, 2022 •

edited

Loading